Make `get` and `set` arguments take identifiers instead of strings #203

ttencate · 2023-03-26T12:29:54Z

Refactors KvParser to accept some expressions, as long as they don't
contain a comma at the top level outside any pair of [], () or {}.

Also tightens up spans in error messages quite a bit.

Prelude for #199.

Bromeon

Thanks a lot, that's quite the refactoring! 💪

Would it be possible to add 1-2 tests with new expressions that were previously unsupported? E.g. Vector2::new(2.5, -1.0) or so. You can also extend existing tests.

bors try

Bromeon · 2023-03-26T13:41:09Z

godot-macros/src/util.rs

+    pub fn handle_any(&mut self, key: &str) -> Option<Option<KvValue>> {
+        self.map.remove(&ident(key))
    }


Why Option<Option>? Should rather be ParseResult, no?

There is no way for this to fail. Updated the doc comments of these handle_* funcs to clarify.

Bromeon · 2023-03-26T13:43:50Z

godot-macros/src/util.rs

+            let errors = self
+                .map
+                .keys()
+                .map(|ident| error(format!("unrecognized key `{ident}`"), ident));
+            Err(errors
+                .reduce(|mut a, b| {
+                    a.combine(b);
+                    a
+                })
+                .unwrap())


This repeats "unrecognized key" N times in the output, no? Why this change, is it not harder to read?

Nicer error spans, pointing to the key in question. The case of multiple unrecognized keys should be pretty rare I think.

Bromeon · 2023-03-26T13:45:43Z

godot-macros/src/util.rs

+struct ParserState<'a> {
+    tokens: std::slice::Iter<'a, TokenTree>,
+    prev: Option<&'a TokenTree>,
+    curr: Option<&'a TokenTree>,


Typical abbreviation for "current" is "cur", see e.g. CurDir.

Bromeon · 2023-03-26T13:49:02Z

godot-macros/src/util.rs

+    fn parse_opt_value(&mut self) -> ParseResult<Option<KvValue>> {
+        Ok(match self.curr {
+            // End of input directly after a key
+            None => None,
+            // Comma following key
+            Some(tt) if is_punct(tt, ',') => {
+                self.next();
+                None
+            }
+            // Equals sign following key
+            Some(tt) if is_punct(tt, '=') => {
+                self.next();
+                Some(self.parse_value()?)
+            }
+            Some(tt) => {
+                return bail("expected `=` or `)`", tt);
+            }
+        })
    }


I repeatedly find myself confused about an all-surrounding Ok(...) and wonder why returning Result in the first place 😬

Could you avoid ? inside Ok(...)? Just split the expressions, like:

let result = match self.cur { ... }; Ok(result)

That makes clear that only if you reach the last line, Ok is being returned.

Bromeon · 2023-03-26T13:54:25Z

godot-macros/src/util.rs

+    impl PartialEq for KvValue {
+        fn eq(&self, other: &Self) -> bool {
+            let to_strings = |kv: &Self| {
+                kv.tokens
+                    .iter()
+                    .map(ToString::to_string)
+                    .collect::<Vec<String>>()
+            };
+            to_strings(self) == to_strings(other)
+        }
+    }


This is a very expensive comparison:

allocate N + M strings, for each token on either side

copy all strings into two vectors

Where is this needed? Is there no way to compare tokens directly?

Also, why PartialEq and not also Eq, is there no total equality?

Neither TokenTree nor TokenStream implements even PartialEq unfortunately.

Note that this is in a #[cfg(test)] block. But I'll move it outside to make it more discoverable, annotate it with #[cfg(test)], and add a comment.

We don't need Eq, whether it's semantically correct or not.

I see. But why move it outside if it's only used inside #[cfg(test)]?

It makes sense to have all trait implementations for a type together in the file, I think. Let me know if you'd prefer it to be moved back, I don't care much either way.

Agree, but in this case it probably makes sense to have "test-only code" together -- since it's not really a trait that can be used elsewhere with the type.

Bromeon · 2023-03-26T13:59:41Z

godot-macros/src/util.rs

+                Some(self.parse_value()?)
+            }
+            Some(tt) => {
+                return bail("expected `=` or `)`", tt);


Error message is a bit less expressive than before:

maybe mention that = or ) are expected after a key

also, , is a possible token after a key

Added some extra code to give a more helpful error in case the preceding expression was nontrivial (more than one token tree). Edit: also reworded the error message to be less about tokens.

Awesome, thanks for clarification 👍

Bromeon · 2023-03-26T14:03:08Z

godot-macros/src/util.rs

        (
            $($key:expr => $value:expr),*
            $(,)?
        ) => {
            {
                let mut map = std::collections::HashMap::new();
                $(
-                    map.insert($key, $value);
+                    map.insert(ident($key), $value);


Nice change! 👍

We can do even better: since the keys are always identifiers, we don't even need the quotes.

bors · 2023-03-26T14:18:59Z

try

Build succeeded:

full-ci

ttencate · 2023-03-27T08:06:24Z

Would it be possible to add 1-2 tests with new expressions that were previously unsupported? E.g. Vector2::new(2.5, -1.0) or so. You can also extend existing tests.

Sure thing, added a couple of tests and rearranged the existing ones for better coverage.

ttencate · 2023-03-27T08:38:49Z

All done in a new commit. Let me know when it's ready to squash.

godot-macros/src/util.rs

Bromeon · 2023-03-27T09:03:11Z

godot-macros/src/util.rs

+    impl PartialEq for KvValue {
+        fn eq(&self, other: &Self) -> bool {
+            let to_strings = |kv: &Self| {
+                kv.tokens
+                    .iter()
+                    .map(ToString::to_string)
+                    .collect::<Vec<String>>()
+            };
+            to_strings(self) == to_strings(other)
+        }
+    }


Agree, but in this case it probably makes sense to have "test-only code" together -- since it's not really a trait that can be used elsewhere with the type.

Bromeon

Let me know when it's ready to squash.

GitHub now has a feature where it lets you compare changes even for force-pushes:

So if that simplifies your workflow, you can keep force-pushing.

Refactors `KvParser` to accept some expressions, as long as they don't contain a comma at the top level outside any pair of `[]`, `()` or `{}`. Also tightens up spans in error messages quite a bit.

Bromeon

Thanks a lot!

bors r+

bors · 2023-03-27T10:06:43Z

Build succeeded:

full-ci

ttencate force-pushed the feature/get_set_without_quotes branch 2 times, most recently from aac0465 to 3fa7b16 Compare March 26, 2023 12:33

Bromeon added feature Adds functionality to the library c: tooling CI, automation, tools labels Mar 26, 2023

Bromeon reviewed Mar 26, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Mar 26, 2023

Try #203:

aa40611

Bromeon reviewed Mar 27, 2023

View reviewed changes

ttencate force-pushed the feature/get_set_without_quotes branch 2 times, most recently from c0a80b3 to 14682fe Compare March 27, 2023 09:14

Make get and set arguments take identifiers instead of strings

1b32155

Refactors `KvParser` to accept some expressions, as long as they don't contain a comma at the top level outside any pair of `[]`, `()` or `{}`. Also tightens up spans in error messages quite a bit.

ttencate force-pushed the feature/get_set_without_quotes branch from 14682fe to 1b32155 Compare March 27, 2023 09:20

Bromeon approved these changes Mar 27, 2023

View reviewed changes

bors bot merged commit c1c2d60 into godot-rust:master Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `get` and `set` arguments take identifiers instead of strings #203

Make `get` and `set` arguments take identifiers instead of strings #203

ttencate commented Mar 26, 2023 •

edited

Loading

Bromeon left a comment •

edited

Loading

Bromeon Mar 26, 2023

ttencate Mar 27, 2023

Bromeon Mar 26, 2023

ttencate Mar 27, 2023

Bromeon Mar 26, 2023

ttencate Mar 27, 2023

Bromeon Mar 26, 2023

ttencate Mar 27, 2023

Bromeon Mar 26, 2023

Bromeon Mar 26, 2023

ttencate Mar 27, 2023 •

edited

Loading

Bromeon Mar 27, 2023

ttencate Mar 27, 2023

Bromeon Mar 27, 2023

Bromeon Mar 26, 2023 •

edited

Loading

ttencate Mar 27, 2023 •

edited

Loading

Bromeon Mar 27, 2023

Bromeon Mar 26, 2023

ttencate Mar 27, 2023

bors bot commented Mar 26, 2023

ttencate commented Mar 27, 2023

ttencate commented Mar 27, 2023

Bromeon Mar 27, 2023

Bromeon left a comment

Bromeon left a comment

bors bot commented Mar 27, 2023

Make get and set arguments take identifiers instead of strings #203

Make get and set arguments take identifiers instead of strings #203

Conversation

ttencate commented Mar 26, 2023 • edited Loading

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttencate Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Mar 26, 2023 • edited Loading

Choose a reason for hiding this comment

ttencate Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors bot commented Mar 26, 2023

try

ttencate commented Mar 27, 2023

ttencate commented Mar 27, 2023

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

bors bot commented Mar 27, 2023

Make `get` and `set` arguments take identifiers instead of strings #203

Make `get` and `set` arguments take identifiers instead of strings #203

ttencate commented Mar 26, 2023 •

edited

Loading

Bromeon left a comment •

edited

Loading

ttencate Mar 27, 2023 •

edited

Loading

Bromeon Mar 26, 2023 •

edited

Loading

ttencate Mar 27, 2023 •

edited

Loading